Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Infinite Geocoders, Support Offline Geocoder #28

Merged
merged 12 commits into from
Apr 2, 2024

Conversation

miles-grant-ibigroup
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup commented Feb 27, 2024

Updates to latest geocoder package, introduces breaking change that allows for UNLIMITED GEOCODERS

When deploying, be sure to disable api gateway caching. It's not longer compatible

@miles-grant-ibigroup miles-grant-ibigroup self-assigned this Feb 27, 2024
Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my first comments.

env.example.yml Outdated

REDIS_HOST: (optional) <insert IP of redis host here>
REDIS_KEY: (optional) <insert redis password here>
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be a JSON array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's stringified!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Can you just tweak:

Suggested change
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>
GEOCODERS: <Stringified JSON Array of OTP-UI `GeocoderConfig`s>

env.example.yml Outdated
REDIS_HOST: (optional) <insert IP of redis host here>
REDIS_KEY: (optional) <insert redis password here>
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>
BACKUP_GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s. Same length as GEOCODERS>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are backup geocoders supposed to be in the same order as the main ones? Is each backup geocoder at a given index supposed to be of the same kind as the main one? If so, would it make sense to write something like:

GEOCODERS:
- main:
    ...
  backup:
    ...
- main:
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense but we're dealing with serverless which loads all these in as env variables. Doing what you suggest is technically possible but would be a huge overhead to implement

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, just tweak the comment:

Suggested change
BACKUP_GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s. Same length as GEOCODERS>
BACKUP_GEOCODERS: <Stringified JSON Array of OTP-UI `GeocoderConfig`s. Same length and order as GEOCODERS>

handler.ts Show resolved Hide resolved
Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the indicated comment tweaks.

env.example.yml Outdated

REDIS_HOST: (optional) <insert IP of redis host here>
REDIS_KEY: (optional) <insert redis password here>
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Can you just tweak:

Suggested change
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>
GEOCODERS: <Stringified JSON Array of OTP-UI `GeocoderConfig`s>

env.example.yml Outdated
REDIS_HOST: (optional) <insert IP of redis host here>
REDIS_KEY: (optional) <insert redis password here>
GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s>
BACKUP_GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s. Same length as GEOCODERS>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, just tweak the comment:

Suggested change
BACKUP_GEOCODERS: <JSON Array of OTP-UI `GeocoderConfig`s. Same length as GEOCODERS>
BACKUP_GEOCODERS: <Stringified JSON Array of OTP-UI `GeocoderConfig`s. Same length and order as GEOCODERS>

handler.ts Show resolved Hide resolved
@miles-grant-ibigroup
Copy link
Collaborator Author

Great suggestions thanks!

Copy link
Collaborator

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, a few suggestions but nothing blocking.

handler.ts Outdated Show resolved Hide resolved
handler.ts Outdated Show resolved Hide resolved
handler.ts Outdated Show resolved Hide resolved
handler.ts Show resolved Hide resolved
@miles-grant-ibigroup miles-grant-ibigroup merged commit 4623ba9 into master Apr 2, 2024
1 check passed
@miles-grant-ibigroup miles-grant-ibigroup deleted the rebuild branch April 2, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants